Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create webhook-related components. #17

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Create webhook-related components. #17

wants to merge 2 commits into from

Conversation

rofrankel
Copy link
Contributor

@rofrankel rofrankel commented Aug 17, 2024

Corresponding guidance added in aep-dev/aeps#209.

@rofrankel rofrankel marked this pull request as ready for review August 17, 2024 00:11
@rofrankel rofrankel requested a review from a team as a code owner August 17, 2024 00:11
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! a few comments

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you also need to add something to schemas/? and json_schema too ideally.

option go_package = "aep.dev/api";

// Provides a generic wrapper for all webhook notifications.
message Notification {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// The payload of the notification.
//
// The type of this payload should directly correspond to the event type.
google.protobuf.Any payload = 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this have the same problem as any other .Any where you don't know the schema? and therefore you need some sort of side-documentation that isn't statically verified like JsonSchema?

seems like a pattern for defining webhook schema would be better than an actual type.

// // This notification is sent when a GroupJoinRequest is created; that is,
// // when a user requests to join a group.
// message BookArchived {
// option (aep.api.webhook_payload) = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOC - how are webhooks supposed to work in gRPC?

I feel like protobufs will only work when your server defines and RPC schema that the client knows how to send requests to - theoretically I guess the gRPC protocol does send over HTTP/2 so you as long as your message is packed via protobuf and the request payload matches someone could send a payload to you.

I suppose that's also the motivation for using protobuf.Any? you can send any payload type you want to and leave it to the webhook received to cast the object appropriately?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants